Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the webpack shortcode config #14485

Merged
merged 1 commit into from
Mar 18, 2019
Merged

Conversation

youknowriad
Copy link
Contributor

closes #14436

The shortcode script has a default export, which means it should be included in the LibraryExportDefaultPlugin webpack config.

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Mar 18, 2019
@youknowriad youknowriad self-assigned this Mar 18, 2019
@youknowriad youknowriad added this to the 5.3 (Gutenberg) milestone Mar 18, 2019
@ellatrix
Copy link
Member

Hm, does that fix it for you? I still have the same error on this branch.

@youknowriad
Copy link
Contributor Author

@ellatrix make sure to re-run npm run dev after switching to this branch.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad Ah, stupid of me. :) It works, but why is this not on for all modules? Any way this can be automatic so we cannot make the same mistake with other packages?

@youknowriad
Copy link
Contributor Author

@ellatrix I don't really know how this plugin works if there's no "default" export, will it create an empty object by default? If It's the case I think it's fine to use it by default for all packages. cc @gziolo and @aduth might know more.

@youknowriad youknowriad merged commit 6e340d6 into master Mar 18, 2019
@youknowriad youknowriad deleted the fix/webpack-shortcode-config branch March 18, 2019 09:03
@gziolo
Copy link
Member

gziolo commented Mar 18, 2019

this["wp"] = this["wp"] || {}; this["wp"]["deprecated"] =
/******/ (function(modules) { // webpackBootstrap
...
/******/ })["default"];
//# sourceMappingURL=index.js.map

It adds this ["default"] stuff at the end of the file. I guess we could tweak it somehow different to avoid maintaining a whitelist.

For reference, a package which doesn't use default:

this["wp"] = this["wp"] || {}; this["wp"]["date"] =
/******/ (function(modules) { // webpackBootstrap
...
/******/ });
//# sourceMappingURL=index.js.map

Something like this might work as well:

"default" in module && module = module["default"]

@ellatrix
Copy link
Member

@gziolo That would be great!

@gziolo
Copy link
Member

gziolo commented Mar 18, 2019

Yes, @aduth made a good point last week. We should optimize the code to avoid any lists of exceptions for packages. I removed one of them from Jest config last week. That should be the next one to fix.

@aduth
Copy link
Member

aduth commented Mar 18, 2019

I'm still curious why this would have suddenly become a regression, if shortcode has always had a default export since July 2018, and LibraryExportDefaultPlugin has never been configured to include 'shortcode'.

@aduth
Copy link
Member

aduth commented Mar 18, 2019

Is it possible that we're running into a conflict between the two versions of wp.shortcode (script handles wp-shortcode vs. shortcode)?

https://github.com/WordPress/wordpress-develop/blob/6d2f78d9bacf931fb9d4ba031e135c4eb5b17713/src/js/_enqueues/wp/shortcode.js

@youknowriad
Copy link
Contributor Author

I'd think it's the opposite. Both scripts supposed to have the exact same API (copy/paste). I think we may have never loaded the Gutenberg shortcode script and started doing so recently.

@aduth
Copy link
Member

aduth commented Mar 18, 2019

@youknowriad Right, I could imagine that being the case. If they're meant to be drop-in replaceable, then I imagine it shouldn't be an issue, but worth keeping an eye on further possible regressions. It makes me wonder if we couldn't also just replace the existing script from core with this new one, if it behaves the same (i.e. keep shortcode script handle for compatibility, but point it to wp-shortcode).

@youknowriad
Copy link
Contributor Author

@aduth Right!, I had a patch at some point, worth revisiting https://core.trac.wordpress.org/ticket/44987

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Classic Block: can no longer insert shortcodes
4 participants